Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update calloop and wayland-rs to the latest versions #404

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

kchibisov
Copy link
Member

Suggested-by: Ian Douglas Scott [email protected]
Fixes #402.

Based on #403, cc @ids1024 @DJMcNab

The calloop wayland source doesn't work and not released yet. Smithay/calloop-wayland-source#1

@kchibisov kchibisov force-pushed the update-wayland-rs branch 2 times, most recently from 0038f1d to 65770c7 Compare September 20, 2023 20:28
@kchibisov kchibisov marked this pull request as ready for review September 20, 2023 20:28
@kchibisov
Copy link
Member Author

Should be good now.

@kchibisov kchibisov force-pushed the update-wayland-rs branch 2 times, most recently from ce68a13 to 62f8691 Compare September 20, 2023 20:30
@@ -652,6 +653,8 @@ impl DataDeviceHandler for DataDeviceWindow {
(o, d, Some(t)) => (o, d, t),
_ => return,
};
// SAFETY: it's safe as long as we don't close the underlying file.
let f: &mut fs::File = unsafe { f.get_mut() };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is annoying, though I'm not sure of a better way to do it here or in calloop while preserving IO safety. (And also not over complicating things; like manual syscalls with rustix instead of using File methods).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've discussed this in #calloop channel, basically the issue is that NoIoDrop thing can't be re-wrapped by us...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, with the alternative of doing a dup, but there's no way I'll do a dup for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dup works, but yeah, not great. Technically also perfectly safe to define a struct implementing Read using rustix::io::read on the fd... but that would be rather verbose if nothing else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm thinking in the case of SCTK, handling pipes from data device would be a place where using the futures/IO adapters from calloop could make quite a bit of sense 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about Futures, but yeah, if we write a specialized object which simply read/writes maybe it'll be nicer in the end, I just ported the API.

Copy link
Member

@ids1024 ids1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make further (breaking API) changes later to use IO safety in public APIs, probably using Rustix, and not need some of the unsafe blocks here. But everything should be good here as far as just updating the dependencies.

@wash2
Copy link
Member

wash2 commented Sep 21, 2023

Should we add a note to the CHANGELOG as part of this PR? I can increment the version and do a release after merging.

@kchibisov
Copy link
Member Author

Hm, sure, I can add.

@kchibisov
Copy link
Member Author

I've added that we basically updated them, since it's a reexport and is a breaking change.

@kchibisov kchibisov requested a review from wash2 September 21, 2023 21:00
@wash2 wash2 merged commit a77fef4 into Smithay:master Sep 22, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

calloop dependency out of date
4 participants